Fix static pages being de-opted into dynamic rendering#86
Fix static pages being de-opted into dynamic rendering#86marlonschlosshauer wants to merge 9 commits into
Conversation
|
@marlonschlosshauer is attempting to deploy a commit to the Pro Team Team on Vercel. A member of the Team first needs to authorize it. |
dougbot-agent
left a comment
dougbot-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution — good direction here.
A few things we need before merging:
- Tests
We need explicit tests covering the rendering-boundary change and the bug + fix. Right now the PR moves a lot of routes and touches several follow-on areas, but there is no failing-first regression coverage showing static routes stay on the static path while auth-dependent routes still work. - Scope
This PR includes the route-group split plus several follow-up fixes that surfaced during static generation (CommunitySortDropdown,NoOpenerLink, theme-provider behavior, header session wiring). Those may all be related, but they make the change broader than a tiny localized fix, so the automated coverage needs to be stronger before we merge it. - Implementation / merge readiness
DCO is currently failing on the head commit, and the PR body already notes some routes were not validated locally. Please fix the signoff issue and update the verification with exact commands once the regression tests are in place.
Once those are addressed, we can take another pass.
|
Status update on the current head:
Once the branch has regression coverage plus a signed-off follow-up commit, I can take another pass. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Re-checked the current PR head The route-group split direction still looks reasonable, but the merge blockers on this head are still the same:
Separate from code review, Vercel is still waiting on team authorization, but that remains a maintainer-side/external check rather than the code blocker here. Once the branch has focused regression coverage for the rendering boundary and a signed-off follow-up commit, I can take another pass. |
2 participants
Summary
Refactored the current app directory setup to only do the server side auth check for pages that potentially need it.
See all the
○and●routes in the build output:Linked Issue or Exception
Closes #85
Root Cause (for bugs)
The
getServerAuthSession()call in the root layout causes Next.js to de-opt all pages into dynamic rendering. However a lot of the pages currently present in the app directory can be static (e.g landing page, about, updates, etc).Solution
Moved the server side auth check closer to where it is actually needed; thus freeing up the other pages to be statically rendered.
Scope
Test Plan
Describe specific scenarios tested: Landing page
Screenshots (if UI)
Checklist
Notes for Reviewers
I decided to split the app directory into
(static)and(dynamic)route groups. Could also be achieved with just a(dynamic)route group.I have not gotten the Shopify or admin routes to run locally, so didn't test those.
Now that sites are generated statically, I had to fix a bunch of issues that materialised:
CommunitySortDropdownin a Suspensenoopener noreferrerlinks into a client componentThemeProviderbe present before mountThese aren't directly related to the splitting of the app directory, but are a result of static generation.
This PR doesn't necessarily change a lot, but it does moves a lot of files. I can totally understand if this kind of optimisation isn't yet what you're looking for.